Skip to content

Test-session resilience: turn-stall watchdog, idle-TTL reaper, lifecycle logs#444

Closed
LikiosSedo wants to merge 5 commits into
mainfrom
fix/kb-test-session-resilience
Closed

Test-session resilience: turn-stall watchdog, idle-TTL reaper, lifecycle logs#444
LikiosSedo wants to merge 5 commits into
mainfrom
fix/kb-test-session-resilience

Conversation

@LikiosSedo

Copy link
Copy Markdown
Collaborator

Summary

Fixes three production-confirmed defects in the read-only test-session path (kbc/platform/pod/compile_box.py), all box-side. Each was verified against the code before fixing.

1. Test turns hung on "thinking" forever (no stall watchdog)

test_session_driver ran a bare async for msg in client.receive_messages(): await _emit_message(...) with no model-stall watchdog — unlike the compile path, which runs _consume_turn_stream under _model_stall_watchdog (armed in _run_wrapper). When the gateway black-holed a model request (connection accepted, no response), the turn stayed active and the UI spun indefinitely with no recovery.

Fix: _test_stall_watchdog + _consume_test_turn_stream reuse the compile idle-latency semantics (any SDK event == alive; a pending read tool relaxes to the longer tool bound so a slow Read/Grep is never false-killed). On a stall the turn is reaped once, without retry: interrupt the SDK stream, emit a turn-level error + turn_done so the UI goes idle, and keep the session live for the next question (the receive loop discards the torn-down ResultMessage). Idle bound defaults to the compile bound, overridable via KBC_TEST_MODEL_IDLE_TIMEOUT_S. Test sessions keep faithful streaming (no de-stream shim), so this watchdog is their only stall guard.

2. Orphaned sessions exhausted the concurrency cap (no idle TTL / list endpoint)

TEST_SESSIONS had no idle TTL and no enumeration. A server-side timeout or a persistence failure left box-side sessions nobody closed, filling KBC_MAX_TEST_SESSIONS (default 3) until the pod was recreated.

Fix: TestRun now tracks created_at / last_activity_at / a monotonic idle marker (touched on open, each turn, and event consumption). A periodic sweep (_test_session_reaper, KBC_TEST_SESSION_IDLE_TTL_S default 1800s) tears down idle sessions and emits a close event. New endpoint GET /test-sessions returns {"sessions":[{"tid","parent_run_id","created_at","last_activity_at","done"}]} — wire contract agreed with the consumer (field names are load-bearing).

3. Zero lifecycle observability

The box only printed its startup line; pod logs showed nothing for test-session activity.

Fix: _print_test_lifecycle emits one stdout line per lifecycle event (test.open / test.close / turn.start / turn.done / turn.stalled / ttl.reap), carrying tid + parent_run_id only — never user content.

Constraints held

  • CompileRun behavior untouched.
  • No changes to destream or unrelated areas; changes are confined to compile_box.py (+ tests).
  • Python style matches the file.

Tests

test_compile_box.py (+3, full suite 69 passing):

  • stall reaper frees a wedged turn, emits error + turn_done, and the same session answers a follow-up question; asserts stdout lines carry tid/parent and leak no question text;
  • idle-TTL sweep drops orphans (snapshot dir + slot freed) and spares an active session;
  • GET /test-sessions returns the agreed wire shape.

…fecycle logs

Three production-confirmed defects in the read-only test-session path
(kbc/platform/pod/compile_box.py), all box-side:

1. Test turns hung on "thinking" forever. The test-session receive loop
   (test_session_driver) was a bare `async for msg in receive_messages()`
   with no model-stall watchdog — unlike the compile path, which runs
   _consume_turn_stream under _model_stall_watchdog. A black-holed model
   request (gateway accepts but never responds) left the turn active and the
   UI spinning indefinitely, with no recovery.

   Fix: _test_stall_watchdog + _consume_test_turn_stream reuse the compile
   idle-latency semantics (any SDK event == alive; a pending read tool relaxes
   to the tool bound). On a stall the turn is reaped once (no retry):
   interrupt the SDK stream, emit a turn-level error + turn_done so the UI goes
   idle, and keep the session live for the next question. The receive loop
   discards the torn-down ResultMessage. Bound defaults to the compile idle
   bound, overridable via KBC_TEST_MODEL_IDLE_TIMEOUT_S.

2. Orphaned sessions exhausted the concurrency cap. TEST_SESSIONS had no idle
   TTL and no way to enumerate sessions, so a server timeout or a persistence
   failure left box-side sessions that nobody closed, filling
   KBC_MAX_TEST_SESSIONS (3) until pod recreation.

   Fix: TestRun now tracks created_at / last_activity_at / a monotonic idle
   marker (touched on open, each turn, and event consumption). A periodic
   sweep (_test_session_reaper, KBC_TEST_SESSION_IDLE_TTL_S default 1800s)
   tears down idle sessions and emits a close event. New GET /test-sessions
   returns {"sessions":[{tid,parent_run_id,created_at,last_activity_at,done}]}
   (wire contract agreed with the consumer).

3. Zero lifecycle observability. The box only printed its startup line, so pod
   logs showed nothing for test-session open/close/turn activity.

   Fix: _print_test_lifecycle emits one stdout line per lifecycle event
   (test.open / test.close / turn.start / turn.done / turn.stalled / ttl.reap),
   carrying tid + parent_run_id only — never user content.

Constraints held: CompileRun behavior untouched; destream/unrelated areas not
touched; test sessions keep faithful streaming (no de-stream shim), so the new
watchdog is their only stall guard.

Tests (test_compile_box.py, +3): stall reaper frees a wedged turn and keeps the
session live for a follow-up question; idle-TTL sweep drops orphans and spares
active sessions; GET /test-sessions returns the agreed wire shape. Full suite
green (69 passing).
…session limit

Wire the box-side test-session resilience through the TS gateway so the consumer
can reach it, aligned with the sicore side.

1. capability.testSessions (src/gateway/server.ts): new RPC method proxying the
   box's GET /test-sessions. Read-only reconciliation — mirrors testClose's box
   discovery and NEVER spawns/rehydrates a box just to list (absent/dead box →
   empty list). Box rows pass through verbatim; the `tid` wire field is
   load-bearing for the consumer and is not renamed. Response:
   {run_id, sessions:[{tid,parent_run_id,created_at,last_activity_at,done}]}.

2. Stable 429 error code (kbc/platform/pod/compile_box.py): the box's
   "too many concurrent test sessions" refusal now returns the structured
   {error:{code,message,retriable:false}} shape (same convention as
   handle_test_recommendation) with code "test_session_limit". The agentbox
   error mapping (agentBoxResponseError → wrapRpcError) passes the code +
   retriable=false through unchanged. Without this a bare 429 mapped to the
   generic TOO_MANY_REQUESTS with retriable=true — indistinguishable from a
   model rate-limit 429 and wrongly retriable. Not retriable by design: the
   caller must close an idle session, not auto-retry.

3. Wire contract (src/gateway/capability/contract.ts): CAPABILITY_TEST_SESSIONS,
   the request/response/summary interfaces, and TEST_SESSION_LIMIT_ERROR_CODE
   documented so the box↔runtime↔consumer contract is single-sourced.

Tests: new src/gateway/server-capability-test-sessions.test.ts (passthrough with
tid preserved / empty on absent box / run_id required); box test asserts the
structured 429 body. tsc --noEmit clean; gateway vitest green; box suite 69
passing.
Two P2 follow-ups from the sicore MR review, layered on the test-session work.

1. testStart idempotency (kbc/platform/pod/compile_box.py + gateway):
   handle_open_test accepts an optional `idempotency_key`. A retried open with
   the same key returns the SAME live session (same tid/snapshot_hash/pages)
   with `idempotent_replay: true`, instead of opening a second session or
   consuming a second concurrency slot — so a lost ack / client retry cannot
   leave a ghost session. Checked before the cap (a replay is never 429'd) and
   before packing (no snapshot work). The key→tid map is process-local and
   cleared with the session on teardown, so a torn-down key opens fresh and can
   never replay a dead tid. Absent key → behavior unchanged (old-consumer compat).
   The gateway's capability.testStart passes the field through and, crucially,
   SKIPS starting a second event relay on an idempotent_replay — the box's
   /test-events is single-consumer, so a duplicate relay would split the stream.

2. Run-scoped listing (gateway): capability.testSessions filters the box's rows
   to the requested run_id. A shared box (SICLAW_COMPILE_BOX_ENDPOINT) hosts
   sessions for multiple parent runs; one run must never see or reap another's.
   Double protection with the consumer's own ParentRunID check.

Field name `idempotency_key` (aligned with sicore). Tests: box test asserts
same-key replay → same tid + replay flag + unchanged active count, different key
→ new session, teardown drops the key (fresh afterwards), no-key never deduped;
gateway test asserts a shared box's two runs are mutually invisible in the list.
tsc --noEmit clean; gateway vitest green; box suite 70 passing.
Follow-up to the testStart idempotency change: the relay-skip guard only fires
on the rare idempotent-replay path, and a future refactor of the testStart
handler could silently drop it — splitting the box's single-consumer
/test-events stream. Extract the decision into a pure predicate
(shouldRelayTestSession) in the relay module and unit-test both branches
(replay → no relay; fresh open or flag absent → relay); the handler now calls
the predicate instead of an inline check. Behavior unchanged. tsc clean;
test-relay vitest green.
Align the testStart idempotency key with sicore (MR!696) and the codebase's
existing convention: every consumer-minted idempotency key here is a `<noun>_id`
(message_id for a turn, command_id for a typed command, operation_id for a
mutation receipt) — there is no `idempotency_key`. So the field is
`client_request_id`, not the `idempotency_key` the first pass used. Behavior is
unchanged: same client_request_id → same live session (checked before the cap and
before packing), empty/absent → open fresh. Box read + TestRun attribute +
teardown cleanup, the gateway request type + passthrough, and the box test's
wire bodies all renamed. box suite 70 green; tsc clean; gateway vitest green.
@LikiosSedo

Copy link
Copy Markdown
Collaborator Author

Consolidated into #446 per owner request; review history stays here. This PR will be closed once the batch merges.

LikiosSedo added a commit that referenced this pull request Jul 23, 2026
…le pods

Integration fix for the #442 × #444 merge interaction. #442 renamed the
compile-box pod prefix (agentbox- → kbc-box-, profile-derived) and updated
onReap/onAdopt to pass rec.profile, but left the other capability call sites
resolving the pod name with the default "agentbox" prefix. On main that was
correct (every box was agentbox-); after the rename those sites silently point
at a pod name the compile box no longer uses, so in K8s mode:

  - capability.testClose reports a LIVE test session as already-closed
  - capability.testSessions (new in #444, which reused testClose's pattern)
    lists no sessions for a live compile run
  - capability.cancel and the ensureCapabilitySession cleanup paths 404 on
    stop() and leak the pod instead of reaping it (ghost cleanup)

Pass the run's profile at every capability-box site so getAsync/stop build the
kbc-box- prefix. The profile comes from the in-memory run record, or — for the
restart-safe paths (testClose/testSessions/cancel) where the record may be gone
— is recovered from the consumer store via adopt(notifyOnAdopt:false), which
never reattaches a relay or spawns a box. Unknown profile falls back to the
default prefix (the pre-rename behavior).

Chat-agent paths (chat.sessionStatus, agent.clearMemory, agent.terminate) are
intentionally left on the default prefix — they operate on chat agentIds, not
capability runs.

Tests: memory-cold testClose/testSessions now assert getAsync is called with
the recovered "kb-compile" profile (→ kbc-box-<id>); cancel asserts stop carries
the profile for both in-memory and store-only runs.
@LikiosSedo

Copy link
Copy Markdown
Collaborator Author

Superseded by #446 (platform hardening batch); branch retained.

@LikiosSedo LikiosSedo closed this Jul 23, 2026
pull Bot pushed a commit to TheTechOddBug/siclaw that referenced this pull request Jul 23, 2026
Box turn-stall watchdog, idle-TTL reaper, test-session list; gateway proxy/idempotency + relay predicate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant